fix(redis-worker): move unrecoverable queue items to the dead-letter queue#4258
fix(redis-worker): move unrecoverable queue items to the dead-letter queue#4258claude[bot] wants to merge 2 commits into
Conversation
…queue SimpleQueue.dequeue() skipped items whose job field was not a string, whose job had no registered schema, or whose payload failed schema validation. Because the queue uses a visibility-timeout redelivery model (items are only removed on ack or by moving them to the DLQ), these skipped items were re-scored and redelivered on every visibility-timeout cycle indefinitely. These failures are unrecoverable, so route all three branches to moveToDeadLetterQueue() instead of silently skipping. Add tests covering the unknown-job and unparseable-payload cases.
🦋 Changeset detectedLatest commit: 6eefa5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -198,6 +198,7 @@ export class SimpleQueue<TMessageCatalog extends MessageCatalogSchema> { | |||
| const parsedItem = JSON.parse(serializedItem) as any; | |||
There was a problem hiding this comment.
Hi. I think the malformed-JSON case still needs the same DLQ treatment becase right now JSON.parse(serializedItem) runs before the new invalid-item checks. And if the queue hash contains a corrupt/non-JSON value, dequeue() will hit the outer catch and throw, so the item can remain in the queue and keep blocking/redelivering instead of moving to the DLQ.
Could you, please, wrap the parse for each item and move that item to the DLQ when parsing fails? A small regression test with a manually seeded items hash value like "not-json" would cover the remaining poison-message path.
sviat-barbutsa
left a comment
sviat-barbutsa
left a comment
There was a problem hiding this comment.
Hi, I left one inline comment about the remaining malformed queue payload case. Please, take a look.
3 participants
Overview
SimpleQueueuses a visibility-timeout redelivery model: a dequeued item is re-scored and left in the sorted set, and it only leaves the queue when it is acked or moved to the dead-letter queue (DLQ).Before: When
SimpleQueue.dequeue()encountered an item it could not process — one whosejobfield was not a string, one whosejobhad no registered schema, or one whose payload failed schema validation — it logged an error and skipped the item. Because nothing removed it, the item was re-scored and redelivered on the next visibility-timeout cycle, and this repeated on every cycle indefinitely (a poison-message loop).After: Each of these three cases is unrecoverable — retrying cannot make the item's job or payload valid — so on first encounter the item is moved to the dead-letter queue and never redelivered. This mirrors how the max-attempts path in
worker.tsalready handles valid-but-failing handlers.How
In
SimpleQueue.dequeue(), the three "invalid item" branches (non-stringjob, missing schema, failedsafeParse) nowawait this.moveToDeadLetterQueue(id, <reason>)beforecontinue, instead of only logging and skipping.moveToDeadLetterQueueremoves the item from the main queue and records its payload and the error in the DLQ. The existing error logs are kept for context.✅ Checklist
Testing
Added tests in
packages/redis-worker/src/queue.test.tscovering the unknown-job and unparseable-payload cases: each asserts the item is moved to the DLQ, the main queue is emptied, and the item does not resurface on a subsequentdequeue().Changelog
SimpleQueue.dequeue()now moves unrecoverable items (invalid job field, unknown job schema, or payload that fails schema validation) to the dead-letter queue on first encounter instead of logging and silently re-queuing them, preventing an indefinite redelivery loop.Screenshots
N/A
💯
Generated by Claude Code